Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Octicon SVG spritemap #10107

Merged
merged 38 commits into from
Feb 11, 2020
Merged

Add Octicon SVG spritemap #10107

merged 38 commits into from
Feb 11, 2020

Conversation

jolheiser
Copy link
Member

@jolheiser jolheiser commented Feb 2, 2020

To Do:

  • Initial pass, swap out octicons for SVGs
  • Go through and fix broken SVGs as noticed
  • Fix outside functionality (JS)

Signed-off-by: jolheiser <[email protected]>
templates/repo/header.tmpl Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 2, 2020
@guillep2k
Copy link
Member

make vendor?

@jolheiser
Copy link
Member Author

jolheiser commented Feb 2, 2020

This PR needs much more work before I look for approvals (sorry, just noticed I never marked as draft or WIP)

Currently I'm just gathering feedback on whether this seems like an acceptable solution. 😄

@jolheiser jolheiser changed the title Add Octicon SVG spritemap WIP: Add Octicon SVG spritemap Feb 2, 2020
@jolheiser
Copy link
Member Author

jolheiser commented Feb 2, 2020

make vendor?

Interestingly, it's not picked up because of the build tag. 🤔

EDIT: Unless you were talking about the CI failure. 😅

Signed-off-by: jolheiser <[email protected]>
Signed-off-by: jolheiser <[email protected]>
Signed-off-by: jolheiser <[email protected]>
Copy link
Member

@guillep2k guillep2k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some comments at the top of the script file with instructions on how to update the icons? I'm guessing it's like:

  • Choose a new version of the library; update go.mod accordingly.
  • make vendor to bring it up to date.
  • Run the script with (insert exact go run command here).
  • git add, commit...

@silverwind
Copy link
Member

Github does publish a npm module for octicons at https://www.npmjs.com/package/@primer/octicons. You could npm install --save @primer/octicons and take the SVGs from the node_modules/@primer/octicons/build/svg directory for the build.

@jolheiser
Copy link
Member Author

@silverwind I was going to do that, but they don't publish a sprite, so we would need to still make one somehow, or use them individually.

While I'm sure it's possible, someone more experienced with JS would likely need to do it.

@jolheiser
Copy link
Member Author

Yet another alternative is to forego the script altogether and manually stitch together a sprite whenever we want to update.

@silverwind
Copy link
Member

The output file could be optimized further with tools like https://github.com/svg/svgo. I guess a make svg step could output to web_src/build (which is in .gitignore) and SVGO CLI could be used for the final optimization step (npx svg <input> <output>) and output to public.

As for spritesheeting, I have used https://github.com/svgstore/svgstore in the past, but there are probably better tools around.

@jolheiser
Copy link
Member Author

@silverwind I am using https://github.com/jkphl/svg-sprite, which apparently uses svgo in the output.

I will commit my changes once I have finished swapping out icons so that people can review. 😃

@silverwind
Copy link
Member

@jolheiser ok. I see fill-rule and title in your output spritesheet. I think those could be eliminated by SVGO, they aren't really necessary.

Signed-off-by: jolheiser <[email protected]>
Signed-off-by: jolheiser <[email protected]>
@jolheiser jolheiser changed the title WIP: Add Octicon SVG spritemap Add Octicon SVG spritemap Feb 5, 2020
@jolheiser
Copy link
Member Author

I think this is more or less ready for review.
There were honestly far fewer mega-octicon than I expected, so if you'd rather I split up the template helper I can.
I considered not using a helper, but since now we need StaticUrlPrefix to access the sprite, I figured it would be easier to make the whole thing a helper.

I will definitely need feedback on the Makefile entry. I didn't include it in the webpack command since it's not likely to be modified often.
The library used to make the sprite claims to use svgo, but other than "minifying" I didn't see much difference.

And of course Drone is mad about PhantomJS, though I am unsure how exactly to resolve that...

@techknowlogick techknowlogick added the topic/ui Change the appearance of the Gitea UI label Feb 5, 2020
@techknowlogick techknowlogick added this to the 1.12.0 milestone Feb 5, 2020
@jolheiser
Copy link
Member Author

That last one seems to have fixed it. 🎉

webpack.config.js Outdated Show resolved Hide resolved
web_src/js/index.js Outdated Show resolved Hide resolved
@silverwind
Copy link
Member

I think you still need to add a entry for icons.svg to templates/pwa/serviceworker_js.tmpl.

Signed-off-by: jolheiser <[email protected]>
@jolheiser
Copy link
Member Author

@silverwind Done with both. I didn't add the {{MD5 AppVer}} to the end, as I doubt the icons will change often.
But I'm also not an expert at the serviceworker, so let me know if I should add it.

@silverwind
Copy link
Member

silverwind commented Feb 7, 2020

{{MD5 AppVer}} there is not a choice and only needed when the actual request has it which is not the case here.

One more related improvement that might be good to have is a preload entry for it, e.g.

<link rel="preload" as="image" href="{{StaticUrlPrefix}}/img/svg/icons.svg">

Signed-off-by: jolheiser <[email protected]>
@jolheiser
Copy link
Member Author

@silverwind Done.

@silverwind
Copy link
Member

Any more reviews here? I'm very excited to get SVG icons in personally 😉

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 11, 2020
@jolheiser
Copy link
Member Author

Ping lgtm

@techknowlogick techknowlogick merged commit 86fdba1 into go-gitea:master Feb 11, 2020
@jolheiser jolheiser deleted the octicon branch February 11, 2020 17:03
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants